Follow symlinks in rooignore checks #7405
Conversation
dosubot
bot
added
the
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| // Follow symlinks to get the real path | ||
| let realPath: string | ||
| try { | ||
| realPath = fsSync.realpathSync(absolutePath) |
There was a problem hiding this comment.
I notice we are using fsSync.realpathSync() which is a synchronous operation. Since validateAccess might be called frequently during file operations, have you considered the performance impact? Would it be worth exploring an async version or perhaps caching resolved paths to minimize the blocking I/O operations?
| } catch { | ||
| // If realpath fails (file doesn't exist, broken symlink, etc.), | ||
| // use the original path | ||
| realPath = absolutePath |
There was a problem hiding this comment.
When realpathSync fails, we silently fall back to the original path. While this maintains backward compatibility, it could mask issues like broken symlinks or permission problems. Would it be helpful to at least log these failures for debugging purposes?
|
|
||
| /** | ||
| * Check if a file should be accessible to the LLM | ||
| * Automatically resolves symlinks |
There was a problem hiding this comment.
The comment mentions Automatically resolves symlinks but does not explain why this is important. Would it help future maintainers to add a brief note about the security implications (TOCTOU prevention)?
|
|
||
| // Ignore expects paths to be path.relative()'d | ||
| // Follow symlinks to get the real path | ||
| let realPath: string |
There was a problem hiding this comment.
Consider extracting this symlink resolution logic into a private helper method for better readability and potential reuse. This would make the main logic cleaner and the helper method could be reused if needed elsewhere.
| expect(controller.validateAccess("node_modules/package.json")).toBe(false) | ||
|
|
||
| // Symlink to ignored file should also be blocked (TOCTOU fix) | ||
| expect(controller.validateAccess("config.json")).toBe(false) |
There was a problem hiding this comment.
Great test for the basic symlink scenario! Would it be valuable to add tests for edge cases like: broken symlinks (where realpathSync would throw), symlink chains (symlink pointing to another symlink), and circular symlinks?
hannesrudolph
added
the
Issue/PR - Triage
daniel-lxs
left a comment
daniel-lxs
left a comment
There was a problem hiding this comment.
LGTM
Tested it and it works
hannesrudolph
added
PR - Needs Review
and removed
Issue/PR - Triage
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
Thanks to @thelicato for flagging this
Important
Enhance
RooIgnoreControllerto resolve symlinks invalidateAccessto prevent TOCTOU attacks, with corresponding tests added.validateAccessinRooIgnoreController.tsnow resolves symlinks to prevent TOCTOU attacks.realpathSyncfails, falls back to the original path.RooIgnoreController.spec.tsto ensure symlinks pointing to ignored files are blocked.fsSync.realpathSyncto simulate symlink resolution.fsSyncimport inRooIgnoreController.tsfor synchronous file operations.This description was created by
for ce9def7. You can customize this summary. It will automatically update as commits are pushed.